Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix get_blobs_list_async method to return BlobProperties #32545

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jul 12, 2023

The get_blobs_list_async method advertised to return list of BlobProperties but returned list of blob names. This is a bug that has been detected by MyPy checks with the new Azure blob package.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

The get_blobs_list_async method advertised to return list of
BlobProperties but returned list of blob names. This is a bug
that has been detected by MyPy checks with the new Azure blob
package.
Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find, looks good +1

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why this bug was not detected in the tests, and if we need to update the tests after this change

@potiuk
Copy link
Member Author

potiuk commented Jul 12, 2023

I'm wondering why this bug was not detected in the tests, and if we need to update the tests after this change

I think as of this new version of azure-blobs, MyPy check is "good enough" for the test case :)

@potiuk
Copy link
Member Author

potiuk commented Jul 12, 2023

I'm wondering why this bug was not detected in the tests, and if we need to update the tests after this change

I think as of this new version of azure-blobs, MyPy check is "good enough" for the test case :)

Philosophical comment: this is basically the "promise" of Type Hinting + MyPY. Once we have all of our code type-hinted (in ideal world) we should be able to drop all the tests that just check if the types are correct. I consider that mostly as "unit testing aid" (as much as hinting the users and helping with auto-completion).

@potiuk
Copy link
Member Author

potiuk commented Jul 12, 2023

Static check and some provider tests passed. Merging.

@potiuk potiuk merged commit a67427a into apache:main Jul 12, 2023
@potiuk potiuk deleted the fix-wasb-get-blob-async-list branch July 12, 2023 07:11
@Adaverse
Copy link
Contributor

One issue not directly related to mypy but can't help noticing that there are other similar functions like get_blobs_list and get_blobs_list_recursive which only return list of blob names. Shall we change those as well to return BlobProperties to maintain consistency? Also, I remember a discussion where a user had raised this - #32258

@potiuk
Copy link
Member Author

potiuk commented Jul 15, 2023

Sure. One can propose it. I looked t at it before, but I wanted to handled the issue at hand where the "advertised" behaviour was inconsistem. IMHO the advertised behaviour inconsistency is an important aspect because you tell something that is not true. The other methods don't tell the type returned (they only tell "list"). so they are not consistent, but at least they are not lying. The goal of this PR fixing a lie (which lead to worsened experience for contributors is worht it), not making things consistent.

But if anyone would like to take on the task of making it consistent - this would be cool and you are welcome to do it :).
If you would like to do so @Adaverse - I am all for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants